Images in XML sitemap are always linked to base store in multistore on Schedule by Nazar65 · Pull Request #19598 · magento/magento2

Nazar65 · 2018-12-06T14:50:07Z

Description (*)

Images in XML sitemap are always linked to base store in multistore on Schedule

Fixed Issues (if relevant)

Images in XML sitemap are always linked to base store in multistore on Schedule #19596: Images in XML sitemap are always linked to base store in multistore on Schedule
...

Manual testing scenarios (*)

Website URL 1: https://my_website.com
Website URL 2: https://my_website.co.uk

Got to Stores->Configuration->Catalog->XML Sitemap->General Settings:
2.Set to Enable, Start Time, Frequency Daily etc...
TIP: I've set to a few minutes before checking!

Website 1 should have images url referencing Website 1 domain = https://my_website.com

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-engcom-team · 2018-12-06T14:50:13Z

Hi @Nazar65. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

sivaschenko

@Nazar65 thanks for the contribution. Please see the code review notes.

sivaschenko · 2018-12-07T09:08:01Z

    /**
+     * @var \Magento\Store\Model\App\Emulation $appEmulation
+     */
+    protected $appEmulation;


please change the access level to private

sivaschenko · 2018-12-07T09:09:16Z

     * @param \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
     */
    public function __construct(
+        Emulation $appEmulation = null,


Please add move the new parameter to the last position in the parameters list

Nazar65 · 2018-12-07T12:14:16Z

@sivaschenko all done.

sivaschenko

@Nazar65 thanks for your updates, please see remaining review notes

sivaschenko · 2018-12-07T13:37:35Z

        \Magento\Framework\Mail\Template\TransportBuilder $transportBuilder,
-        \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
+        \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation,
+        Emulation $appEmulation


This new parameter should be optional

Same here if i add optional, as this have been in older commit the codeMess test will fail.
Backward Compatibility Policy is not applied to Plugins, Observers and Setup Scripts.

The class Observer has a coupling between objects value of 13.

sivaschenko · 2018-12-07T13:37:47Z

            try {
+                $this->appEmulation->startEnvironmentEmulation(
+                    $sitemap->getStoreId(),
+                    'frontend',


Please use the constant

This Class have many dependency, so if i will use constant the codeMess test will fail.

In this case it's better to decompose the class in favour to dropping constant usage

Co-Authored-By: Nazar65 <nazarn96@gmail.com>

Nazar65 · 2018-12-07T13:53:08Z

@sivaschenko please look at comments.

sivaschenko · 2018-12-07T13:57:51Z

Hi @Nazar65 , I think the best way to resolve the problems with static tests and keep the good code style and backward compatibility - is extracting all the business logic to a model from the observer. Actually, this observer is a very good candidate for refactoring. Could you extract it?

Nazar65 · 2018-12-07T14:03:44Z

@sivaschenko yes i agree with you. I'm will work on this :)

# Conflicts: # app/code/Magento/Sitemap/Model/Observer.php

Nazar65 · 2018-12-11T12:00:09Z

Hi @sivaschenko can you please look at newly commit ?

sivaschenko

Hi @Nazar65 , thanks for your refactoring! It looks great, however, I have some imporvement suggestions. Please see my code review notes.

sivaschenko · 2018-12-11T14:41:45Z

     */
    const XML_PATH_CRON_EXPR = 'crontab/default/jobs/generate_sitemaps/schedule/cron_expr';

-    /**


Please keep all existing constants in the Observer as they could be already used by third-party extensions.

sivaschenko · 2018-12-11T14:44:48Z

+     * @var \Magento\Store\Model\StoreManagerInterface
     */
-    protected $_transportBuilder;
+    protected $_storeManager;


I'd change all properties to private and remove the "_" prefix (for consistency and to be compatible with PSR)

sivaschenko · 2018-12-11T14:45:58Z

-            $transport->sendMessage();
-
-            $this->inlineTranslation->resume();
+                $this->sitemapEmail->sendErrorEmail($errors);


Please call this method after the foreach (as all the errors have to be accumulated in $errors first)

sivaschenko · 2018-12-11T14:48:45Z

+            self::XML_PATH_ERROR_RECIPIENT,
+            ScopeInterface::SCOPE_STORE
+        );
+        if ($errors && $recipient) {


I'd move this condition to the observer or to a separate method, to keep this method solid

sivaschenko · 2018-12-11T14:53:02Z

+     *
+     * @var \Magento\Framework\App\Config\ScopeConfigInterface
+     */
+    private $_scopeConfig;


Property should not be prefixed with a single underscore, see https://www.php-fig.org/psr/psr-2/#42-properties

sivaschenko · 2018-12-11T14:53:48Z

@@ -0,0 +1,109 @@
+<?php


Please use strict mode in for all new PHP files

sivaschenko · 2018-12-11T14:56:17Z

-        \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
+        ScopeConfigInterface $scopeConfig,
+        CollectionFactory $collectionFactory,
+        StoreManagerInterface $storeManager,


Neither of two classes appears to be using store manager, so I think this dependency can be removed from both of them

sivaschenko · 2018-12-11T15:02:05Z

+        if ($errors && $recipient) {
+            $this->inlineTranslation->suspend();
+
+            $this->_transportBuilder->setTemplateIdentifier(


I think it may be beneficial to wrap the email send operation in a try-catch block to gracefully handle situations when a mail server is not responsive.

There is an example here

magento2/app/code/Magento/Contact/Model/Mail.php

Lines 68 to 87 in f77f96b

$this->inlineTranslation->suspend();

try {

$transport = $this->transportBuilder

->setTemplateIdentifier($this->contactsConfig->emailTemplate())

->setTemplateOptions(

[

'area' => Area::AREA_FRONTEND,

'store' => $this->storeManager->getStore()->getId()

]

)

->setTemplateVars($variables)

->setFrom($this->contactsConfig->emailSender())

->addTo($this->contactsConfig->emailRecipient())

->setReplyTo($replyTo, $replyToName)

->getTransport();

$transport->sendMessage();

} finally {

$this->inlineTranslation->resume();

}

However, I would improve the approach by catching an exception and writing it to an exception log

sivaschenko · 2018-12-11T15:07:10Z

+/**
+ *  Sends emails for the scheduled generation of the sitemap file
+ */
+class SitemapSendEmail


I'd rename the class to the something like EmailNotification or Mail and the method to send or sendErrors to avoid duplicate wording in full method reference. Currently it's Magento\Sitemap\Model\SitemapSendEmail::sendErrorEmail

Nazar65 · 2018-12-12T12:43:48Z

Hi @sivaschenko all done. :)

sivaschenko

Hi @Nazar65 thanks for the refactoring! I've added some code suggestions. You can simply add all my suggestions to batch and commit them.

Nazar65 · 2018-12-12T15:45:54Z

@sivaschenko all done, thank you for the review :)

sivaschenko · 2018-12-12T16:09:52Z

Thank you @Nazar65 finally there are currently 14 commits, would you like to squash them into a single one?

Nazar65 · 2018-12-12T20:11:50Z

@sivaschenko sorry for that, now i "squash" to one commit.

magento-engcom-team · 2018-12-12T22:39:23Z

Hi @sivaschenko, thank you for the review.
ENGCOM-3683 has been created to process this Pull Request

Nazar65 · 2019-01-17T09:00:54Z

@sivaschenko @sidolov so do I change the variables again?

magento-engcom-team · 2019-01-17T16:13:21Z

Hi @sidolov, thank you for the review.
ENGCOM-3683 has been created to process this Pull Request

Nazar65 · 2019-02-07T10:14:22Z

HI @sivaschenko @sidolov any updates ?

sivaschenko · 2019-02-07T18:06:10Z

Hi @Nazar65 the pull request is in queue for delivery to mainline

ghost · 2019-02-13T15:46:50Z

Hi @Nazar65, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

… multistore on Schedule #19598

fix issue-19596 Images in XML sitemap

d5b0ffd

magento-engcom-team added Component: Sitemap Release Line: 2.3 labels Dec 6, 2018

sivaschenko requested changes Dec 7, 2018

View reviewed changes

sivaschenko self-assigned this Dec 7, 2018

Fix failed static tests

16f11f7

sivaschenko requested changes Dec 7, 2018

View reviewed changes

Update app/code/Magento/Sitemap/Model/Observer.php

c8afe9f

Co-Authored-By: Nazar65 <nazarn96@gmail.com>

Nazar65 added 3 commits December 11, 2018 13:54

Refactoring Sitemap Observer

fda07a2

Merge remote-tracking branch 'origin/issue-19596' into issue-19596

5c7527e

# Conflicts: # app/code/Magento/Sitemap/Model/Observer.php

Refactoring Sitemap Observer

d2d04b6

sivaschenko requested changes Dec 11, 2018

View reviewed changes

Refactoring

b88fa48

Refactoring

85a2a56

sivaschenko requested changes Dec 12, 2018

View reviewed changes

sivaschenko approved these changes Dec 12, 2018

View reviewed changes

magento-engcom-team added this to the Release: 2.3.1 milestone Dec 12, 2018

ENGCOM-3683: Unit test fix.

a04cbf2

magento-engcom-team added the Progress: accept label Dec 21, 2018

sidolov requested changes Jan 4, 2019

View reviewed changes

Comment thread app/code/Magento/Sitemap/Model/Observer.php

sidolov added the Progress: needs update label Jan 4, 2019

sivaschenko removed the Progress: needs update label Jan 17, 2019

sidolov approved these changes Jan 17, 2019

View reviewed changes

Comment thread app/code/Magento/Sitemap/Model/Observer.php

magento-engcom-team assigned sidolov Jan 17, 2019

okorshenko removed this from the Release: 2.3.1 milestone Jan 28, 2019

ghost mentioned this pull request Feb 7, 2019

Magento 2 sitemap wrong paths in sitemap index when running on cron #19565

Closed

magento-engcom-team mentioned this pull request Feb 13, 2019

Images in XML sitemap are always linked to base store in multistore on Schedule #19596

Closed

magento-engcom-team merged commit a04cbf2 into magento:2.3-develop Feb 13, 2019

magento-engcom-team pushed a commit that referenced this pull request Feb 13, 2019

ENGCOM-3683: Images in XML sitemap are always linked to base store in…

cb08046

… multistore on Schedule #19598

magento-engcom-team added the Award: bug fix label Feb 13, 2019

sidolov added this to the Release: 2.3.2 milestone Feb 13, 2019

Nazar65 mentioned this pull request Feb 14, 2019

[Backport]-issue-195196 Images in XML sitemap are always linked to base store in multistore on Schedule #21220

Closed

4 tasks

	$this->inlineTranslation->suspend();
	try {
	$transport = $this->transportBuilder
	->setTemplateIdentifier($this->contactsConfig->emailTemplate())
	->setTemplateOptions(
	[
	'area' => Area::AREA_FRONTEND,
	'store' => $this->storeManager->getStore()->getId()
	]
	)
	->setTemplateVars($variables)
	->setFrom($this->contactsConfig->emailSender())
	->addTo($this->contactsConfig->emailRecipient())
	->setReplyTo($replyTo, $replyToName)
	->getTransport();

	$transport->sendMessage();
	} finally {
	$this->inlineTranslation->resume();
	}

Conversation

Nazar65 commented Dec 6, 2018

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

magento-engcom-team commented Dec 6, 2018

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Dec 7, 2018

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Dec 7, 2018

Uh oh!

sivaschenko commented Dec 7, 2018

Uh oh!

Nazar65 commented Dec 7, 2018

Uh oh!

Nazar65 commented Dec 11, 2018

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment